Remove animations from various places for users with prefer-reduced-motion#3972
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3972 +/- ##
=======================================
Coverage 87.26% 87.26%
=======================================
Files 279 279
Lines 23934 23924 -10
Branches 6325 6317 -8
=======================================
- Hits 20885 20877 -8
+ Misses 2809 2807 -2
Partials 240 240
Continue to review full report at Codecov.
|
julienw
left a comment
julienw
left a comment
There was a problem hiding this comment.
Thanks!
I looked at the code and tried the deploy preview, and I think most "animation" uses are now removed. Except maybe one:
- the SVG icon when uploading
res/img/svg/sharing-animated-dark-12.svg. But maybe that's OK because it's fairly small.
However I can see a lot of transition uses. Some of them are not problematic because they're small, but I can see a few that would be good to be removed:
- in the menu buttons, when the panel is dismissed
- the component showing up when doing drag and drop from a file
- The transition when reordering tracks
- The introduction on the search input
- Transitions in the active tab view, when opening tracks
- Possibly the transition when the "+" button appears when doing a selection
- The help about scrolling in Viewport
I'm totally fine with touching those in other PRs.
| /* Do not animate the publish animation for stripes at the top loading bar. */ | ||
| @media (prefers-reduced-motion) { | ||
| .menuButtonsPublishUploadBarInner { | ||
| animation: none; |
There was a problem hiding this comment.
Optional: in general I think I'd rather have @media (prefers-reduced-motion: no-preference) that adds the animation instead of @media (prefers-reduced-motion) { removing it. I think it makes it easier to maintain in the longer term, reducing the risk of typos.
There was a problem hiding this comment.
When I was adding them, I wanted to follow this logic:
- If a CSS class is added for solely that animation/transition, then wrap that class with
@media (prefers-reduced-motion: no-preference) - If a CSS class is not added for that animation and includes other properties, I added
@media (prefers-reduced-motion)to reset the animations.
I think 2nd item makes it cleaner, but happy to discuss for a follow-up if you want.
There was a problem hiding this comment.
You can use the same class both in @media and outside @media, I think it's OK to specify the same class twice (actually you're doing it with the solution 2 as well!), and on the long run it will be easier to maintain.
For example, if we change the class name in the future, it's easy to forget to update the "negative" @media query (that is, one that removes the animation) because most of us don't use the app with prefers-reduced-motion on. However with the "positive" @media query it's easier to see that the animation is missing when not using prefers-reduced-motion.
Here is my rationale :-)
There was a problem hiding this comment.
Thanks for the review!
Thanks! I looked at the code and tried the deploy preview, and I think most "animation" uses are now removed. Except maybe one:
- the SVG icon when uploading
res/img/svg/sharing-animated-dark-12.svg. But maybe that's OK because it's fairly small.
I'm not sure about it. Indeed, it could be too small and not that disturbing, but I think it's nice to double check with the original reporter since I'm not really sure.
However I can see a lot of
transitionuses. Some of them are not problematic because they're small, but I can see a few that would be good to be removed:
in the menu buttons, when the panel is dismissed
the component showing up when doing drag and drop from a file
The transition when reordering tracks
The introduction on the search input
Transitions in the active tab view, when opening tracks
Possibly the transition when the "+" button appears when doing a selection
The help about scrolling in Viewport
I'm totally fine with touching those in other PRs.
From what original reporter expressed, I got the impression that simple transitions with fade-in/fade-out effects might not be a problem. I think it's mostly related to elements moving around and when they do not scroll with the other part of the page. But I'm using a lot of "maybe"s in these sentences :) It would be definitely good to check with the person who is having this issue. I will file a seperate issue for the things you listed so we can check.
Filed #3973 for that.
| /* Do not animate the publish animation for stripes at the top loading bar. */ | ||
| @media (prefers-reduced-motion) { | ||
| .menuButtonsPublishUploadBarInner { | ||
| animation: none; |
There was a problem hiding this comment.
When I was adding them, I wanted to follow this logic:
- If a CSS class is added for solely that animation/transition, then wrap that class with
@media (prefers-reduced-motion: no-preference) - If a CSS class is not added for that animation and includes other properties, I added
@media (prefers-reduced-motion)to reset the animations.
I think 2nd item makes it cleaner, but happy to discuss for a follow-up if you want.
canova
force-pushed
the
prefer-reduced-motion-animations
branch
from
a8b4b68 to
f32ec59
Compare
2 participants
Fixes #3969 and some more animations we have.
This PR removes the animation from a few places for users with
prefer-reduced-motion:Deploy preview / Production
You can activate the
prefer-reduced-motionby following the steps here: #3969 (comment)